-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CognitiveServices - HealthInsights] SDK for JS #26268
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @joheredi Could you help sign off the api view for this two packages ? Thanks
"sdk-type": "client", | ||
"author": "Microsoft Corporation", | ||
"version": "1.0.0-beta.1", | ||
"description": "A generated SDK for HealthInsightsCancerProfilingRest", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would split this to be "Health Insights Cancer Profiling Rest Library"
const apiKey = process.env["HEALTH_INSIGHTS_API_KEY"] || ""; | ||
|
||
function printResults(cancerProfilingResult: OncoPhenotypeResultOutput): void { | ||
if (cancerProfilingResult.status === "succeeded") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use !isUnexpected instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joheredi I don't think isUnexpected could work here, isUnexpected would only check the status(aka HTTP status code) of the raw response. Here the status is the polling status located in body
of the final response.
@reutgross Generally RLC would not throw exceptions if the final response status code of polling is 200 when calling the getLongRunningPoller
no matter what the polling status is, Succeeded or Failed. But if you only care about the succeeded
result you could set the resolveOnUnsuccessful
, and then the result cancerProfilingResponse
would be successful one.
try {
const poller = await getLongRunningPoller(client, initialResponse, {resolveOnUnsuccessful: false});
const cancerProfilingResponse = await poller.pollUntilDone();
} catch (e) {
// exceptions would be thrown when the polling status is not `succeeded`
}
const endpoint = process.env["HEALTH_INSIGHTS_ENDPOINT"] || "https://eastus.api.cognitive.microsoft.com"; | ||
const apiKey = process.env["HEALTH_INSIGHTS_API_KEY"] || ""; | ||
|
||
function printResults(cancerProfilingResult: OncoPhenotypeResultOutput): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think about these samples as the entry point for new customers to your service. The samples should be easy to read and thoroughly documented.
Can you at least add documentation about what the sample is doing, please? If possible to restructure the body of the sample to reduce the nesting, that would be great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function printResults(cancerProfilingResult, printPrefix: string): void {
if (cancerProfilingResult === null || cancerProfilingResult === undefined) {
return;
}
for (const key of Object.keys(cancerProfilingResult)) {
if (cancerProfilingResult[key]) {
if (typeof cancerProfilingResult[key] !== "object") {
console.log(`${printPrefix} ${key}: ${cancerProfilingResult[key]}`);
} else {
printResults(cancerProfilingResult[key], printPrefix + " " + key);
}
}
}
}
@joheredi how do you think if we print the result like this way ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @qiaozha .
Thanks for proposing a new way of printing the values .
However, I think that in this sample we would like to demonstrate how to access/iterate over the patients and inferences.
Also to demonstrate how to iterate over the evidence of each inference.
The idea behind this sample is not just dumping the values.
I think that the current sample demonstrates this hierarchy better, even if it's not classic in dumping key, value pairs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, please try to add as much comments in the sample as possible
API change check APIView has identified API level changes in this PR and created following API reviews. azure-rest-health-insights-cancerprofiling |
# Conflicts: # common/config/rush/pnpm-lock.yaml
### Packages impacted by this PR Health Insights CancerProfilingRest and ClinicalMatchingRest (new packages). ### Issues associated with this PR [[Cognitive Services - Health Decision Support] REST API Review · Issue #5731 · Azure/azure-rest-api-specs-pr (github.com)](Azure/azure-rest-api-specs-pr#5731) ### Describe the problem that is addressed by this PR ### What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen? ### Provide a list of related PRs _(if any)_ [[Cognitive Services - Health Insights] CADL revision for public preview by asaflevi-ms · Pull Request #22990 · Azure/azure-rest-api-specs (github.com)](Azure/azure-rest-api-specs#22990) --------- Co-authored-by: Asaf Levi <asaflevi@microsoft.com> Co-authored-by: Mary Gao <yanmeigao1210@gmail.com>
Packages impacted by this PR
Health Insights CancerProfilingRest and ClinicalMatchingRest (new packages).
Issues associated with this PR
[Cognitive Services - Health Decision Support] REST API Review · Issue #5731 · Azure/azure-rest-api-specs-pr (github.com)
Describe the problem that is addressed by this PR
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Provide a list of related PRs (if any)
[Cognitive Services - Health Insights] CADL revision for public preview by asaflevi-ms · Pull Request #22990 · Azure/azure-rest-api-specs (github.com)